fix(install): programmatically clean gga clone dir to avoid collision#961
fix(install): programmatically clean gga clone dir to avoid collision#961decode2 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesInstaller temp cleanup
Sequence Diagram(s)Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
The intent is right but I don't think this fixes #31 as shipped. The Windows branch already had a shell-level cleanup with -ErrorAction SilentlyContinue when the issue was filed, so the likely root cause is a removal that fails silently (locked file, AV handle, permissions), and this adds a second cleanup that fails silently in exactly the same way: the os.RemoveAll error is discarded. If removal fails, git clone reproduces the identical error.
Three items before this can move: surface or handle the RemoveAll error instead of discarding it (return it, log it, or retry), add a behavior-level test that creates a stale non-empty gentleman-guardian-angel directory and asserts it is actually gone after resolution (today the tests only compare the returned command strings), and reconsider doing the deletion inside resolveGGAInstall. That function is a pure resolver called from tests and any future dry-run path; deleting at resolution time is an undocumented side effect, and the existing table test now touches real OS temp dirs on every run. Executing the cleanup as part of the returned command sequence, or at execution time with an injectable path, keeps the resolver pure and the tests hermetic.
Push the update and I'll re-review.
2 participants
Summary
Fixes the GGA agent installation collision issue where \git clone\ fails because the target temporary directory \gentleman-guardian-angel\ already exists and is not empty from a previous install attempt.
Problem
When reinstalling or upgrading, the installer clones GGA into the temporary directory \gentleman-guardian-angel. If a previous installation attempt left files in that directory, \git clone\ fails with exit code 128:
\
fatal: destination path 'C:\Users\user\AppData\Local\Temp\gentleman-guardian-angel' already exists and is not an empty directory.
\`n
Solution
Programmatically clean the destination directory before cloning by calling \os.RemoveAll()\ inside
esolveGGAInstall\ at resolution time. This ensures the target directory is completely gone before the clone starts, while maintaining compatibility with the existing shell cleanup commands asserted in the unit tests.
Closes #31
Summary by CodeRabbit